Desk Review — NOT READY (changes required)

CI: ✅ all required checks green (Build, Lint, Format, Type check, Knip, Unit, UI, Browser, Migrations, Storybook, SAST/Opengrep, CodeQL, all 16 Playwright platform shards + web/docs). Skipped jobs are fork-PR only; Trivy is NEUTRAL. No red, no pending.

The styling/feedback polish is well done — hover/focus-visible/active/drag states, valid design tokens (border-border-strong, bg-bg-elevated, accent-base, ring-offset-background), behavior-preserving uploadFile extraction, sound object-URL lifecycle, and a corner remove-button polish. Security is fine: the client file.type check is UX-only and the server action (convex/branding/file_actions.ts) re-validates auth + MIME allowlist + 2 MB cap + safe paths.

🚫 Blocking — missing test for the new drag-and-drop behavior

This PR adds genuinely new interactive logic with branches but no test exercises any of it:

• happy: drop a valid image → uploadFile runs (image-upload-field.tsx L135–144)
• edge: drop while isUploading → ignored (L139)
• error: drop a non-image (!file.type.startsWith('image/')) → silently ignored (L141)

There is no image-upload-field.test.tsx (its siblings branding-form, branding-preview, color-picker-input all have one). branding-form.test.tsx mocks this component, and the e2e settings-depth.spec.ts branding block tests only the brand-color field — neither drives upload or drag-drop.

This fails the repo's own DoD:

• AGENTS.md L77 — "Tests carry the change: unit (happy + one edge + one error) … — or N/A."
• AGENTS.md L195 — "Every feature and fix carries its test — happy path, one edge, one error condition minimum."
• Ripple Map L90 ("A new interactive UI element") → a11y + manual guide + e2e.

The new WithImage Storybook story is a visual variant only — it asserts no behavior.

To clear: add image-upload-field.test.tsx covering (a) valid image dropped → upload invoked, (b) drop while uploading → no second upload, (c) non-image dropped → no upload, plus an accessibility describe block via checkAccessibility() (house pattern). Pure styling changes don't need tests; the drag-drop logic does.

Non-blocking polish (optional)

• Drag-highlight flicker: the <img>/<Image> (L184–200) and empty-state <Plus> (L206) lack pointer-events-none (only the overlay <span> has it, L201). Dragging across a child fires dragleave on the button and briefly clears isDragging; dragover restores it → possible flicker.
• Stuck drag ring: handleDragLeave has no isUploading guard, so isDragging can stay true (ring visible) if a click-upload starts mid-drag, until the next dragleave.
• Type-check inconsistency: drop path filters on file.type.startsWith('image/') (L141) but the <input> path doesn't; a .ico with empty reported MIME would work via picker yet be silently rejected on drop.
• Convention nits: focus-visible:ring-1 ring-offset-2 vs the house ring-2 ring-offset-2; off-scale bg-foreground/55 (L201).

Verdict: NOT READY — add the drag-and-drop component test (the single gating item); the polish notes are optional.

Desk Review — PR #2113 · fix(platform): polish logo/favicon upload button hover and feedback (#1961)

Verdict: NOT READY — CHANGES REQUIRED (one blocking test gap; everything else is sound)

Branch tale/xs7ds2dhms1xdcb48gx82qn195896m5n. Reviewed the whole implementation, not just the diff. The change is genuinely good: both acceptance criteria are met, CI is green, and the existing tests pass. The single blocking item is a missing test for the one piece of new logic the PR introduces.

CI state

gh pr checks 2113 → 45 pass, 0 fail, 0 pending, 5 skipping (fork-PR/conditional jobs only: Validate images, Docs container test, Smoke test, Trivy, Opengrep OSS). Key gates green: Build platform, UI, Type check, Lint, Format, Knip, all Playwright (platform 1–16 / web / docs).

Tests

Ran the project's own suite once, locally, from services/platform:
bunx vitest --run --project client app/features/settings/branding/components/image-upload-field.test.tsx
→ Test Files 1 passed (1), Tests 5 passed (5) (renders accessible control; drop happy path; drop ignored while uploading; non-image drop rejected; axe a11y audit).

What's verified good

• AC1 (visible hover/focus/active feedback): MET. Hover (hover:border-border-strong hover:bg-bg-elevated, group-hover replace overlay, Plus-icon color), focus (focus-visible:ring-ring ring-2 ring-offset-2, group-focus-visible overlay), active (active:scale-[0.97]), and drag highlight (isDragging && border-accent-base ring-accent-base) are all wired to real interactive states, each with motion-reduce: fallbacks.
• AC2 (matches design system): MET. Every utility resolves to a real @tale/ui token — confirmed in packages/ui/src/globals.css: --color-bg-elevated, --color-border-strong, --color-accent-base, --color-ring all defined (light+dark). No silently-no-op classes. The interaction cluster mirrors the DS buttonVariants base.
• Correctness/robustness: object-URL lifecycle is sound (revoked on re-upload, error, remove, and unmount cleanup — no leak found); drop-while-uploading is guarded (if (isUploading) return) plus disabled on the button; pointer-events-none on inner img/overlay/icon prevents drag-highlight flicker; keyboard upload still works (visible <button> is focusable; hidden input tabIndex=-1); aria-label present on button, remove button, and hidden input.
• Security boundary: mimeType/size are validated server-side in convex/branding/file_actions.ts (requireBrandingAdmin, mimeToExtension, MAX_IMAGE_SIZE_BYTES = 2MB), so the client gates are convenience, not a trust boundary.

BLOCKING — must fix

1. Untested new logic: the isAcceptedImage extension-fallback branch — services/platform/app/features/settings/branding/components/image-upload-field.tsx:21-25. The whole reason this helper exists (per its own comment) is to accept a valid file whose browser MIME is blank/non-image/* by falling back to the extension (e.g. a .ico reporting empty type). The tests cover only the image/png (MIME-true) and .txt/text/plain (both-false) cases — the middle branch (MIME not image/* AND name ends in an accepted extension → accepted) has zero coverage, so a regression that drops the fallback would keep all 5 tests green. Add a test in image-upload-field.test.tsx: drop makeFile('favicon.ico', '') (or 'application/octet-stream') and assert mockSaveImage/onUpload fire. (Verified by grepping the test file — no .ico/empty-MIME case exists.)

Recommended (non-blocking)

2. Drop path accepts more types than the picker or server allow — image-upload-field.tsx:21-22. isAcceptedImage returns true for ANY image/* (gif, bmp, avif, tiff…), but the picker's accept is .png,.svg,.jpg,.jpeg,.webp,.ico and the server's mimeToExtension only maps png/svg/jpeg/webp/ico (convex/branding/file_utils.ts:105). Dropping e.g. image/gif passes the client gate, then the server rejects it and the error is silently swallowed (see add --chat-max-width CSS variable #3) — confusing no-op. Consider validating the drop against the same accept list as the picker. The code comment claiming picker and drop are "in sync" is inaccurate.
3. Upload error is swallowed silently — image-upload-field.tsx:109-115. The catch resets preview/URL but never logs or rethrows; a failed upload gives the user (and console) no signal. This violates the repo rule in .claude/CLAUDE.md ("Never use an empty catch block — log or re-throw"). NOTE: pre-existing — the diff relocated this block byte-identically from handleFileChange into uploadFile; it is not newly introduced. Worth a one-line console.error while the code is open, and ideally a user-facing error state, but not a regression from this PR.
4. No tests for the upload-error path, the file-picker onChange path, or drag-over/leave — the shared uploadFile core is exercised via drop, but these branches are untested. Minor for a UI-polish PR.
5. Minor DS nit: main button uses focus-visible:ring-2 while the DS buttonVariants and the remove button (line 225) use ring-1. Pick one for consistency.

NOT READY — CHANGES REQUIRED

1. Add a test covering the isAcceptedImage extension-fallback branch in image-upload-field.test.tsx — drop a file whose MIME is empty/non-image/* but whose name ends in an accepted extension (e.g. favicon.ico) and assert the upload proceeds. This is the only branch of the PR's new logic with no coverage today.

Desk review — #1961 logo/favicon upload polish

Verdict: READY TO MERGE. CI is fully green and the change meets both acceptance criteria.

Verification

• CI: all required checks pass — Build platform, Type check, Lint, Format, Knip, UI, Unit, Storybook, Smoke test, Scan/Trivy/Opengrep, and all 16 Playwright (platform) shards + Playwright (web). The only non-pass entries are skipping (fork-PR / path-filtered jobs), which is expected.
• Tests (run locally): bunx vitest --run --config vitest.ui.config.ts app/features/settings/branding/components/image-upload-field.test.tsx → 6 passed (6).
• Acceptance criteria: ✅ visible hover/focus/active feedback (hover border-border-strong/bg-bg-elevated, focus-visible ring, active:scale, drag accent-base ring, hover/focus replace-overlay); ✅ styling matches the design system — every token (border-border-strong, bg-bg-elevated, accent-base, ring) is defined in packages/ui/src/globals.css for light + dark.

Implementation quality

• Drag-and-drop is added cleanly via an extracted uploadFile callback shared with the picker. The in-flight guard (if (isUploading) return) is correct — useCallback deps include isUploading, so the closure stays fresh (confirmed by the concurrent-drop test).
• isAcceptedImage() correctly mirrors the picker's accept list and is case-insensitive (name.toLowerCase()). The drop path is actually more defensive than the existing picker onChange, which validates nothing.
• New tests cover the new behavior well: drop happy path, concurrent-drop guard, extension fallback, non-image rejection, and an axe a11y audit.

Non-blocking observations (optional follow-ups, not required to merge)

1. Silent upload-failure — the catch {} in uploadFile clears the preview with no toast/log. This is pre-existing (identical try/catch existed in the prior handleFileChange) and out of this PR's scope, but worth a future ticket: e.g. a blank-MIME .ico accepted by the drop gate is rejected by the backend (saveImage derives the extension from MIME only; mimeToExtension('') → throws Unsupported image MIME type), and the user sees the preview silently vanish. In practice browsers usually report image/x-icon, which the backend accepts.
2. DRY nit — ACCEPTED_IMAGE_TYPES (string) and ACCEPTED_EXTENSIONS (array) duplicate the list; the string could be derived via ACCEPTED_EXTENSIONS.join(',').
3. Redundant aria-label on the hidden, tabIndex={-1} file input (the visible button already carries the label).

None of these block merge.